Skip to content

fix(graph): cap /banks/{bank_id}/graph response size to keep Control Plane parseable on dense banks#1605

Closed
connorblack wants to merge 2 commits into
vectorize-io:mainfrom
connorblack:fix/graph-endpoint-default-cap
Closed

fix(graph): cap /banks/{bank_id}/graph response size to keep Control Plane parseable on dense banks#1605
connorblack wants to merge 2 commits into
vectorize-io:mainfrom
connorblack:fix/graph-endpoint-default-cap

Conversation

@connorblack
Copy link
Copy Markdown
Contributor

@connorblack connorblack commented May 13, 2026

Summary

Cap the limit parameter on GET /v1/default/banks/{bank_id}/graph server-side at 200. Edge count scales quadratically with node count for densely-linked banks, and the Control Plane (Next.js) deserializes the response as a single JS string which V8 caps at 0x1fffffe8 (~512 MiB).

Repro

On a 14k-memory bank with ~835k memory_links and 17k entities:

limit response size Control Plane outcome
100 6 MiB ✓ renders
200 23 MiB ✓ renders
500 147 MiB borderline — Next.js parse stalls
1000 (Control Plane default) 596 MiB ✗ V8 string-length error: "Cannot create a string longer than 0x1fffffe8 characters" → UI shows "Server Error: Failed to fetch graph data"

The Control Plane's useState(1000) in data-view.tsx sets the initial fetchLimit, which falls within the API's old limit: int = 1000 signature. Smaller banks fit; banks past ~10k highly-linked memories silently break the UI.

What changes

limit = min(max(1, limit), 200)

A single min(max(...)) clamp inside the api_graph handler. Smaller queries pass through unchanged; large ones clamp to 200 with no error response. UI continues to function on dense banks instead of erroring out entirely.

Why 200

  • 200 nodes is past the practical readable density limit for any 2D graph viz (force-directed layouts get unusable past ~150 nodes regardless).
  • 23 MiB is well within Next.js / V8 parse capacity.
  • Smaller banks where the user requests >200 still get a usable graph; they were never going to hit the V8 limit anyway.

Why server-side

The UI fix would be a one-line change to data-view.tsx's useState(1000) → useState(200). Either works; server-side caps protect SDK and direct API consumers too without depending on the UI staying in sync. Open to swapping for a UI-only fix if maintainers prefer.

Real fix

This is a guard, not a solution. Long-term the graph endpoint should support pagination or streaming so high-density banks can be explored without truncation. Filing this as a stop-gap so the Control Plane stops 500'ing on production banks at scale.

Test plan

  • Verified clamp behavior at limits 10, 200, 1000 against a 14k-memory bank — clamped responses are well-formed GraphDataResponse JSON.
  • No behavior change for limit ≤ 200 requests.
  • pre-commit hooks (ruff check --fix, ruff format, ty check) all green.

@connorblack
fix(graph): cap /banks/{bank_id}/graph limit at 200 to keep responses…
c5f93ba 
… Node-parseable

The graph endpoint accepts ``limit`` and the Control Plane UI defaults to 1000.
Edge count scales quadratically with node count for densely-linked banks
(memory_units cross-linked via memory_links). Empirical measurement on a
14k-memory bank: limit=200 → 23 MiB, limit=500 → 147 MiB, limit=1000 → 596 MiB.

Next.js deserializes the response as a single JS string via response.json();
V8 caps string length at 0x1fffffe8 (~512 MiB). Banks past that cap error out
with "Cannot create a string longer than 0x1fffffe8 characters" and the UI
shows "Failed to fetch graph data" instead of any graph.

Server-side clamp at 200 keeps the API usable for high-volume banks while
preserving response payload semantics for smaller ones (where 200 is well
under typical graph sizes anyway). Graph viz tools are unusable past ~200
nodes regardless, so this isn't a UX regression.

Real fix is paginated/streamed graph loading in the UI; this is a guard
to prevent the existing UI from breaking on large banks until that lands.
Copilot AI review requested due to automatic review settings May 13, 2026 02:09
Copilot AI reviewed May 13, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Caps the server-side limit parameter for GET /v1/default/banks/{bank_id}/graph to prevent extremely large graph responses that can exceed V8/Next.js parsing limits on dense banks, keeping the Control Plane UI usable at scale.

Changes:

  • Clamp limit in api_graph to the range [1, 200] to bound response size.
  • Add inline rationale/comments explaining why the cap is needed (dense-link quadratic edge growth + V8 string cap).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hindsight-api-slim/hindsight_api/api/http.py Outdated
Comment thread hindsight-api-slim/hindsight_api/api/http.py Outdated
@connorblack
fix(graph): align signature default with clamp + add clamping test
f74b5f1 
Address Copilot review on vectorize-io#1605:

1. Move limit signature default from 1000 to 200 via Query(...) so OpenAPI
   docs match runtime behavior. ge=1 added for explicit lower bound. The
   silent clamp at 200 is preserved (no 422) so existing UI/SDK callers
   passing limit > 200 keep working with a smaller response. Description
   on the Query field documents the cap and rationale.

2. Add tests/test_graph_endpoint_clamp.py:
   - test_graph_limit_above_cap_is_silently_clamped: assert request with
     limit=1000 returns 200 OK and response.limit == 200, nodes <= 200.
   - test_graph_limit_below_cap_passes_through: assert request with
     limit=50 returns 200 OK and response.limit == 50 (no modification).
@nicoloboschi
Copy link
Copy Markdown
Collaborator

nicoloboschi commented May 25, 2026

Closing this. After reviewing, the server-side clamp masks the symptom but creates a new one: the Control Plane's data-view.tsx ships useState(1000) for fetchLimit with a "Load more" button that bumps by +1000 per click. With this clamp, "Load more" silently no-ops past 200 and the "Showing 200 of 14000" counter sticks — same broken UX, different shape.

The 596 MiB payload isn't really driven by node count either. Skimming memory_engine.get_graph_data:

  • Direct links from memory_links are already capped at 10k server-side.
  • observation_inferred_links (memory_engine.py:4880-4913) is uncapped and O(N²) per shared entity. On a bank with 17k entities and dense observation co-occurrence, this is almost certainly where the bulk of the payload comes from — not the 1000 nodes.
  • The endpoint also returns full table_rows (full text, context, tags, ISO dates per row) on the same payload as the graph. list_memory_units already exists for that.
  • Node payloads also carry full text (line 4853) when only the truncated label is needed for viz.

Better path, in priority order:

  1. Enable GZipMiddleware — JSON of repeated keys compresses ~10×, single biggest win for ~3 lines.
  2. Cap observation_inferred_links at 10k like the direct links — one-line fix for the quadratic explosion.
  3. Drop text/context from node payloads; thin them to id/label/color/entity-count.
  4. Add include_table=false and have data-view.tsx call list_memory_units separately.
  5. Long-term: viewport-driven fetching (seed + N-hop neighborhood) since force-directed layouts are unusable past ~150 nodes anyway.

Going to start with (1) on a fresh branch, then profile a dense bank to confirm where the bytes actually go before chasing (2)-(4).

@nicoloboschi nicoloboschi mentioned this pull request May 25, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@connorblack @nicoloboschi